Skip to content

fix: determine package version even in deactivated venv#387

Merged
jenstroeger merged 3 commits intostagingfrom
fix-makefile-package-version
Nov 26, 2022
Merged

fix: determine package version even in deactivated venv#387
jenstroeger merged 3 commits intostagingfrom
fix-makefile-package-version

Conversation

@jenstroeger
Copy link
Copy Markdown
Owner

@jenstroeger jenstroeger commented Nov 17, 2022

Thank you @behnazh for noticing the regression with v2.4.1, introduced by PR #346. The problem is (still and again 🤦🏻‍♂️) that we don’t set up a venv in a Github runner and therefore we shouldn’t rely on the VIRTUAL_ENV environment variable.

This change expands the Python command so that the command always returns a package version string (which may be "unknown") instead of failing the import package.

I think it’s safe to expect "unknown" for make venv (or its Action equivalent)

- name: Create empty virtual environment for Actions
run: mkdir .venv

and make setup

- name: Install dependencies
run: make setup

because the package hasn’t been installed yet, and therefore its version can’t be determined.

However, after make setup the package’s version can be determined safely—with or without virtual environment. For example, the artifacts attached to workflow run 3488321082 indeed contain the correct package version number again:

pkg

@jenstroeger jenstroeger requested a review from behnazh November 17, 2022 06:49
Comment thread Makefile Outdated
@jenstroeger jenstroeger force-pushed the fix-makefile-package-version branch from 97378b7 to 30641a0 Compare November 17, 2022 12:27
@jenstroeger jenstroeger force-pushed the fix-makefile-package-version branch from 30641a0 to 66d2e0d Compare November 17, 2022 12:37
@jenstroeger
Copy link
Copy Markdown
Owner Author

@behnazh after our conversation today we could add after these two lines

# Set the package's name and version for use throughout the Makefile.
PACKAGE_NAME := package
PACKAGE_VERSION := $(shell python -c $$'try: import $(PACKAGE_NAME); print($(PACKAGE_NAME).__version__);\nexcept: print("unknown");')

the following

ifeq ($(PACKAGE_VERSION),unknown)
  $(warning Unable to determine package version, proceeding anyway)
endif

That way, a user shouldn’t be surprised anymore if "unknown" sneaks into file names.

@behnazh
Copy link
Copy Markdown
Collaborator

behnazh commented Nov 18, 2022

@behnazh after our conversation today we could add after these two lines

# Set the package's name and version for use throughout the Makefile.
PACKAGE_NAME := package
PACKAGE_VERSION := $(shell python -c $$'try: import $(PACKAGE_NAME); print($(PACKAGE_NAME).__version__);\nexcept: print("unknown");')

the following

ifeq ($(PACKAGE_VERSION),unknown)
  $(warning Unable to determine package version, proceeding anyway)
endif

That way, a user shouldn’t be surprised anymore if "unknown" sneaks into file names.

Shouldn't we exit with a failure when package version is "unknown"?

@jenstroeger
Copy link
Copy Markdown
Owner Author

jenstroeger commented Nov 18, 2022

Shouldn't we exit with a failure when package version is "unknown"?

Thing is that "unknown" is valid when running make venv and make setup. We could add more checks to exclude these two goals, or issue a warning instead of error & exit.

@behnazh
Copy link
Copy Markdown
Collaborator

behnazh commented Nov 20, 2022

We could add more checks to exclude these two goals, or issue a warning instead of error & exit.

No, I think the code as it is captures the issues and we don't need more warnings.

@jenstroeger jenstroeger merged commit 95026b4 into staging Nov 26, 2022
@jenstroeger jenstroeger deleted the fix-makefile-package-version branch November 26, 2022 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants